-
Notifications
You must be signed in to change notification settings - Fork 202
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Task: refactor extensions file #5160
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
json rpc server alignments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thewahome Am I correctly assuming there we no changes in the logic?
async function updateStatusBarItem(context: ExtensionContext, kiotaStatusBarItem: StatusBarItem): Promise<void> { | ||
try { | ||
const version = await getKiotaVersion(context, kiotaOutputChannel); | ||
if (!version) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we improve this, even in a separate issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No changes to the logic
if (!version) { | ||
throw new Error("kiota not found"); | ||
} | ||
kiotaStatusBarItem.text = `$(extensions-info-message) kiota ${version}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kiota not kiota?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's from a refactor. The strings were carried as they were. I am not sure if changing them here would affect the localization downstream
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I appreciate the much needed cleanup effort here, I'd like for us to hold merging this until the target PR gets merged to main.
A couple of reasons for that:
- we're on tight deadlines to get the preview out.
- additional functional changes need to happen in the main PR, and we don't want additional delays to deal with potential conflicts.
- major refactoring right before releasing are a risky endeavour
Does that make sense?
@baywet |
Quality Gate failedFailed conditions See analysis details on SonarCloud Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
Overview
Closes #5140
Notes
extension.ts
file into individual commands that implement the base Command class